C#: Remove expanded assignments. by michaelnebel · Pull Request #21452 · github/codeql

michaelnebel · 2026-03-11T12:12:21Z

This PR has two goals, primarily to avoid making multiple upgrade/downgrade scripts:

Remove expanded assignments. This aligns our implementation with Java and should make adoption of the shared CFG library easier. It also simplifies expression handling and avoids caching a large predicate. Previously, when the extractor encountered a += b, it would synthesize a = a + b.
Swap the left- and right-hand-side child indices for assignments. This lets us remove several hacks/rotations currently used in the code.

Review on a commit-by-commit basis is encouraged.

Implementation notes

The implementation now “correctly” classifies a += b (e.g., when a and b are int) as an operator call, since it implicitly invokes the user-defined static operator on Int32.
The only compound assignment that does not call an operator is ??= as the null-coalescing operator is a built-in short-circuit like operation.
Added a new set of classes to represent expressions involving a specific operation (e.g., AddOperation, which can represent either a + b or a += b).

DCA notes

Performance appears unchanged.
The missing result for cs/unsynchronized-getter is due to removal of a false positive: the query did not properly account for expanded assignments.
The missing result for cs/web/xss looks acceptable: one alert is removed, but it is essentially a shorter path to the same underlying alert (which is here)
The new results for cs/useless-upcast look acceptable.
Remaining differences appear consistent with normal DCA background noise.

csharp/ql/lib/semmle/code/csharp/ExprOrStmtParent.qll

michaelnebel · 2026-03-20T13:05:19Z

@hvitved : I would really appreciate a review of this PR even though it is still in draft mode. The PR still needs upgrade/downgrade scripts and more DCA runs.

aschackmull · 2026-03-23T10:56:50Z

csharp/ql/lib/semmle/code/csharp/exprs/Assignment.qll

 * An assignment operation. Either an arithmetic assignment operation
- * (`AssignArithmeticOperation`), a bitwise assignment operation
- * (`AssignBitwiseOperation`), or an event assignment (`AddOrRemoveEventExpr`).
+ * (`AssignArithmeticOperation`), a bitwise assignment operation or


Spurious "or"

Suggested change

* (`AssignArithmeticOperation`), a bitwise assignment operation or

* (`AssignArithmeticOperation`), a bitwise assignment operation

aschackmull · 2026-03-23T11:03:25Z

csharp/ql/lib/semmle/code/csharp/exprs/Assignment.qll

+}

+/**
+ * An assignment operation that corresponds to an operator call, for example `x += y` corresponds to `x = x + y`.


This reads somewhat off to me. Did you mean something like this instead?

Suggested change

* An assignment operation that corresponds to an operator call, for example `x += y` corresponds to `x = x + y`.

* An assignment operation that corresponds to an operator call, for example `x += y` corresponds to a call to `+=`.

Some further elaboration on this piece of qldoc might also be nice to clarify exactly which compound assignments are considered OperatorCalls.

aschackmull · 2026-03-23T11:05:26Z

csharp/ql/lib/semmle/code/csharp/exprs/Assignment.qll

+/**
+ * An assignment operation that corresponds to an operator call, for example `x += y` corresponds to `x = x + y`.
+ */
+class AssignCallOperation extends AssignOperation, OperatorCall, @assign_op_call_expr {


Now that most AssignOperations extend OperatorCall, then the qldoc for OperatorCall needs a tweak as it currently claims to only cover calls to user-defined operators.

Yes, we should consider changing the wording as I am not sure what a non user-defined operator is (unless it is a real built-in like &&). The operator used in a += b (when a and b are of type int) is a user-defined operator and it is declared here.

aschackmull · 2026-03-23T11:06:14Z

csharp/ql/lib/semmle/code/csharp/exprs/Assignment.qll

+ * An assignment operation that corresponds to an operator call, for example `x += y` corresponds to `x = x + y`.
+ */
+class AssignCallOperation extends AssignOperation, OperatorCall, @assign_op_call_expr {
  override string toString() { result = "... " + this.getOperator() + " ..." }


This toString can be simplified.

Suggested change

override string toString() { result = "... " + this.getOperator() + " ..." }

override string toString() { result = AssignOperation.super.toString() }

aschackmull · 2026-03-23T12:03:40Z

csharp/ql/lib/semmle/code/csharp/exprs/Operation.qll

+import Expr
+
+/** A binary operation that involves a null-coalescing operation. */
+abstract private class NullCoalescingOperationImpl extends BinaryOperation { }


I believe all the classes in this file break some new ground wrt the AST class hierarchy design, so perhaps ought to be considered in a slightly broader bike-shedding session. Or is there something similar in other languages that I'm not aware of?
Regardless, I'm not a big fan of all the Add-prefixed names. Sure they're private, but they're still not very nice and they overload the meaning of "Add" in a context that already references "Add". Also, it's unfortunate that e.g. AddExpr and AssignAddExpr are not subclasses of AddOperation - that would be more natural. And if the latter is fixed, then I don't think we need all those addition-to-abstract-class classes that have such awkward names, which would address the former point as well.

The other option that I considered was extending the DB-scheme with @add_operation = @add_expr | @assign_add_expr; and similar for the other operations, then the QL part will look more "natural". Would you prefer such a solution instead?

aschackmull · 2026-03-23T12:05:28Z

csharp/ql/lib/semmle/code/csharp/ExprOrStmtParent.qll

-   * ```
+   * Use `expr_parent` instead.
   */
  cached


May as well get rid of cached.

We can't, it's a public predicate in a cached module - so it has to be cached as well.

Just remove it; the QL doc on the file says INTERNAL: Do not use..

aschackmull · 2026-03-23T13:24:58Z

Besides the nits, code changes generally LGTM.

hvitved

Overall LGTM; no further comments than what @aschackmull has already pointed out, thanks for making this change.

aschackmull · 2026-03-24T09:26:13Z

csharp/ql/lib/upgrades/e73ca2c93df8aae162f1704edc4817a5cb330529/rotate_assignments.ql

+  // Swap children for assignments, local variable declarations and add/remove event.
+  if isAssignment(parent)
+  then
+    child = 0 and expr_parent(e, 1, parent)


These two need new_expressions(parent, _, _) and new_expression(e, _, _) to prevent db inconsistencies, right?

This query predicate is only concerned with contracting and rotating the parent/child relation.
The first case handles rotating the children of "assignments" (non-expanding), where we re-use existing expression entities (and the expressions are included in the update to the expressions table, which is the result of running new_expressions - however, I haven't tried running the code yet).

It's exactly the contraction that I'm thinking about - i.e. currently the child-parent tuples for the expressions that we delete in new_expressions are being kept, which I think will create an inconsistent db.

aschackmull · 2026-03-24T09:29:54Z

csharp/ql/lib/upgrades/e73ca2c93df8aae162f1704edc4817a5cb330529/rotate_assignments.ql

+      child = 1 and expr_parent(e, 0, op)
+    )
+    or
+    expr_parent(e, child, parent) and not exists(getOperatorCall(parent))


Same here, I believe.

aschackmull · 2026-03-24T09:32:41Z

What about downgrade script?

michaelnebel · 2026-03-24T09:49:54Z

What about downgrade script?

Uh, sorry - I should have mentioned that the PR is now considered "draft" again. I haven't even tried running the upgrade script yet - and yes - a downgrade script it also needed - but I would like to test the upgrade script first.

…ignments.

…se operations are operator calls.

…o extend OperatorCall and add AssignOperation classes.

…ct expanded assignments.

…nments (those that was previously covered by expanded assignments).

…ove hack that rotates children of assignments.

github-actions bot added the C# label Mar 11, 2026

michaelnebel force-pushed the csharp/expandedassignment branch from c94de85 to 1d8d712 Compare March 11, 2026 12:47

github-advanced-security bot found potential problems Mar 11, 2026

View reviewed changes

csharp/ql/lib/semmle/code/csharp/ExprOrStmtParent.qll Fixed Show fixed Hide fixed

michaelnebel force-pushed the csharp/expandedassignment branch 9 times, most recently from abb75be to 5021ea9 Compare March 16, 2026 10:00

michaelnebel changed the title ~~C#: Cleanup expanded assignments.~~ C#: Remove expanded assignments. Mar 16, 2026

michaelnebel force-pushed the csharp/expandedassignment branch 13 times, most recently from 41fef59 to 07144c2 Compare March 20, 2026 12:09

michaelnebel requested a review from hvitved March 20, 2026 13:18

michaelnebel force-pushed the csharp/expandedassignment branch from 07144c2 to d2188dd Compare March 23, 2026 09:09

aschackmull reviewed Mar 23, 2026

View reviewed changes

hvitved reviewed Mar 23, 2026

View reviewed changes

michaelnebel force-pushed the csharp/expandedassignment branch 2 times, most recently from fd781ef to 8f725cd Compare March 24, 2026 08:57

aschackmull reviewed Mar 24, 2026

View reviewed changes

michaelnebel added 15 commits March 24, 2026 15:31

C#: Don't extract expanded assignments and swap child indices for ass…

c8169f5

…ignments.

C#: Update the DB scheme to reflect that assign arithmetic- and bitwi…

b426c6f

…se operations are operator calls.

C#: Update the child indices for assignments, update Assign classes t…

327757d

…o extend OperatorCall and add AssignOperation classes.

C#: Deprecate the expanded assignment predicate as we no longer extra…

2a78183

…ct expanded assignments.

C#: Cleaup expanded assignments from the dispatch logic.

e2afb00

C#: Introduce a new kind of assignable definitions for compound assig…

569e33b

…nments (those that was previously covered by expanded assignments).

C#: Update the CFG implementation based on the new operations and rem…

149df86

…ove hack that rotates children of assignments.

C#: Upgrade libraries and queries to use the new Operation classes.

5167331

C#: Add/update tests and expected output.

5551634

C#: Add operation types to the DB scheme.

96f55fb

C#: Use the DB types and replace the abstract class implementation.

3d2d09d

C#: Adress review comments.

a900fe8

C#: Remove expr_parent_adjusted.

d96e8cb

C#: Add upgrade script.

e458163

C#: Add downgrade script.

028f2b8

michaelnebel force-pushed the csharp/expandedassignment branch from 8f725cd to 028f2b8 Compare March 24, 2026 14:32

	* (`AssignArithmeticOperation`), a bitwise assignment operation or
	* (`AssignArithmeticOperation`), a bitwise assignment operation

	* An assignment operation that corresponds to an operator call, for example `x += y` corresponds to `x = x + y`.
	* An assignment operation that corresponds to an operator call, for example `x += y` corresponds to a call to `+=`.

	override string toString() { result = "... " + this.getOperator() + " ..." }
	override string toString() { result = AssignOperation.super.toString() }

Conversation

michaelnebel commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

michaelnebel commented Mar 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelnebel Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschackmull commented Mar 23, 2026

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschackmull commented Mar 24, 2026

Uh oh!

michaelnebel commented Mar 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michaelnebel commented Mar 11, 2026 •

edited

Loading

michaelnebel Mar 23, 2026 •

edited

Loading